R4 P0: C2S 门禁设计收口与吸收清单验真 - #1892

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Central review

Decision: request_changes
Reviewed head: 83f56d37f2837bd3bdc2dd930a98b22667330827
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Resolve and wire coffin ownership before declaring P0 closed

docs/plan-refactor-c2s-gate-v1.md:149 · strict-maintainability

P0 is marked complete as a design closeout, but matrix row 27 still specifies CoffinBreak authorization as the unresolved alternative "owner 或明确公共破坏策略". The absorbed source plan confirms this is a real unresolved A/B product decision (docs/plans-skeleton/plan-bughunt-coffin-reclaim-owner-gate-v1.md:58-64). Production state also cannot currently support the proposed owner gate: server/src/coffin/mod.rs:113-122 defines CoffinEntity without any owner identity, CoffinRegistry::insert at lines 132-145 accepts and records no owner, and reclaim_occupied at lines 200-214 reconstructs a coffin without one. Nevertheless, this plan classifies the issue as fully absorbed at line 244 and commits P1 to connecting all 14 fully absorbed items at lines 286-288. Consequently P1 has neither a frozen authorization rule nor an authoritative owner producer/lifecycle to consume; an implementation can choose incompatible break behavior or cannot implement OwnershipRule at all while still appearing to follow the completed P0 matrix.

Root cause: the plan declares design closure and full absorption before resolving the coffin-break ownership policy and before specifying the domain state that produces authoritative coffin ownership, leaving a required production gate disconnected from its source of truth.

[major] Bound gate-denial alerts and logging per client

docs/plan-refactor-c2s-gate-v1.md:99 · performance

The new P0 contract requires P1 to emit an EventAlert for every gate rejection (docs/plan-refactor-c2s-gate-v1.md:99) and to log every rejection (:97), but defines no per-player request or denial budget, coalescing, or backpressure anywhere in the plan. The real ingress path processes every queued bong:client_request event in handle_client_request_payloads (server/src/network/client_request_handler.rs:541), deserializes each payload (:557), and has no request throttle; repository search finds rate limiting for chat only, not this channel. A connected client can repeatedly send a valid-version request that deterministically fails a cheap gate such as OutOfReach or TargetNotFound. Following this frozen design makes every input perform gate work, append a log record, serialize an S2C payload, and enqueue an alert packet. Thus request volume directly creates unbounded per-tick CPU, log output, and outbound packets, allowing one client to drive tick latency and output queues without reaching any gameplay handler. Existing validation does not prevent this because version-valid requests are accepted and denial is precisely the new response path; the PR introduces/exposes it by making per-denial feedback and logging part of the completed P0 contract and P1 acceptance criteria.

Root cause: the frozen gate contract treats every rejected c2s message as an independently logged and acknowledged event but omits resource bounds at the untrusted request boundary. p1 needs an explicit per-client budget/backpressure policy, with denial alerts and logs rate-limited or coalesced, plus a flood/burst acceptance test that proves bounded work and output.

[major] Frozen Euclidean reach contract breaks existing Workbench boundary behavior

docs/plan-refactor-c2s-gate-v1.md:61 · correctness

The new frozen contract requires every player-to-world-target reach check to use 3D Euclidean squared distance and assigns Workbench a 3.0 radius. The current production helper server/src/craft/workbench.rs:73-81 deliberately uses Chebyshev distance, and its regression test at server/src/craft/workbench.rs:388-390 explicitly accepts a player at offset [3.0, 3.0, 3.0]. Under the proposed contract that same established input has distance_squared = 27 > 9 and is rejected. The nearby statement at line 74 that P1 must preserve existing gameplay radii does not prevent this because changing the metric changes the accepted region even when the numeric radius remains 3.0. This PR turns the skeleton's previously open metric question into the contradictory completed P0 contract, so a subsequent conforming implementation would introduce the behavior change.

Root cause: the plan treats preserving a numeric radius as preserving reach behavior, but freezes a global euclidean metric without preserving domain-specific existing metrics. for workbench, replacing the established chebyshev predicate with euclidean distance shrinks the accepted interaction volume and contradicts the plan's behavior-preservation requirement.

[major] Update the master and R6 contracts to the 104-variant baseline

docs/plan-refactor-c2s-gate-v1.md:17 · strict-maintainability

The promoted plan states that the authoritative enum now has 104 variants and that the old 113 count "cannot continue". However, the parent plan still defines completion as "113 C2S variants" in docs/plans-skeleton/plan-refactor-master-v1.md:101, and the R6 plan still requires positive/negative samples for "113 C2S" variants in docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:25. Those are live acceptance contracts consumed by later phases, not historical prose. Thus the same repository now has incompatible authoritative completion counts: R4/P3 derives 104 from the enum while the master and dependent R6 gates require 113. No implementation or test can simultaneously demonstrate both exact contracts without ignoring one document or inventing nine variants.

Root cause: the pr corrected a duplicated canonical variant count only in the promoted r4 plan, leaving the parent and cross-track acceptance contracts stale. this creates an already-observable inconsistency in duplicated sources of truth and makes plan completion evidence ambiguous.

[major] P1 cannot wire the exhaustive gate before P3 declares the remaining variants

docs/plan-refactor-c2s-gate-v1.md:286 · wiring

The plan freezes an exhaustive ClientRequestV1::gate_spec() in which every enum variant must return Spec or a reasoned NoGate and says the gate runs on the production path before dispatch (lines 45-67 and 76-89). However, P1 says to install that production gate while wiring only group A's 14 requests plus one migration sample, while P3 is deferred to a later PR to declare all variants (lines 286-292). Rust cannot compile the promised exhaustive method with the other current variants omitted. Making those variants compile via temporary NoGate arms would route requests that this same matrix identifies as requiring gates around the mutation barrier, such as inventory and session requests, and would defeat the no-default-bypass contract until P3. Existing downstream checks do not supply the promised unified pre-dispatch barrier, which the plan itself identifies as currently absent.

Root cause: the staged delivery splits installation of an enum-exhaustive production dispatcher from declaration of most of the enum's required routing metadata, even though those two pieces must be introduced atomically for the production gate to be both compilable and fail-closed.

[major] Target-first rejection ordering exposes an entity existence and location oracle

docs/plan-refactor-c2s-gate-v1.md:84 · security

The frozen evaluation order resolves the attacker-supplied target before checking dimension and distance (docs/plan-refactor-c2s-gate-v1.md:82-89), and the contract says only the first failure is returned (:91). Requests such as WorkbenchOpen, ContainerOpen, NpcInspectRequest, and GiveDanToElder carry client-controlled protocol entity IDs, which production resolves through the global EntityManager::get_by_id path (for example server/src/network/client_request_handler.rs:2450). The plan then defines distinct TargetNotFound, WrongDimension, and OutOfReach reasons (docs/plan-refactor-c2s-gate-v1.md:97) and explicitly maps them to distinguishable requester-visible messages such as “目标不在此界” and “距离太远” (:99). An untrusted client can therefore probe guessed/sequential entity IDs: an absent ID yields TargetNotFound, an existing entity in another dimension yields WrongDimension, and an existing same-dimension entity outside reach yields OutOfReach. Neither the proposed authoritative lookup nor the later ownership/state checks establish that the requester is allowed to know the target exists, so those later checks do not prevent the oracle. This exposure is introduced by the PR's newly frozen gate ordering and rejection contract and contradicts its stated non-disclosure guarantee.

Root cause: the gate design treats global target existence, dimension, and distance failures as requester-visible diagnostics before establishing a visibility/capability boundary for the target. distinct early failures turn authoritative target resolution into an entity-existence and location oracle. the contract should collapse all pre-authorization target failures to one indistinguishable external response (while retaining internal diagnostics), or establish requester visibility/authority before emitting differentiated reasons.

[major] Completed P0 claims an enum-matrix script check that is not present

docs/plan-refactor-c2s-gate-v1.md:284 · testing

The new plan marks P0 complete at line 282 and then states at line 284 that a script compared all 104 matrix rows with all 104 ClientRequestV1 variants with no omissions or duplicates. However, this PR only adds the plan document and deletes its skeleton; no comparison script, test, output, or other reproducible evidence is added. Repository searches find no gate-matrix sweep/comparison implementation, while the actual enum remains separately maintained in server/src/schema/client_request.rs:35. A matrix row can therefore be renamed, duplicated, or omitted while all existing tests still pass, despite the completed phase claiming that exact regression protection/evidence.

Root cause: p0 is declared complete based on an asserted enum-to-matrix script check, but the change does not provide the claimed check or any durable result. the plan and rust enum remain independent sources, so the stated no-gap/no-duplicate acceptance evidence is not reproducible or regression-protected.

Central review

Decision: request_changes
Reviewed head: e90a8ae566e32ca65f9fc468626ef19230850ec3
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Wire the enum-to-matrix contract check into automated tests

docs/plan-refactor-c2s-gate-v1.md:299 · testing

The PR defines the changed P0 contract as matrix variants = ClientRequestV1 variants and says scripts/check_c2s_gate_matrix.py provides its regression check, but the checker is only referenced by this plan and is not invoked by any test or workflow. The repository's E2E workflow runs server, schema, client, agent, smoke, and bot tests, but no matrix checker. A concrete regression—adding a compiling ClientRequestV1 variant while omitting its matrix row (or deleting/reordering a row)—therefore leaves every existing automated check green unless someone manually runs the script, despite the plan's stated automatic inclusion requirement.

Root cause: the new consistency assertion was implemented only as an opt-in standalone script and was not registered in ci or an automated test target, so the observable enum-to-matrix contract has no enforced regression protection.

[major] Unit or tuple request variants silently bypass the matrix check

scripts/check_c2s_gate_matrix.py:12 · correctness

enum_variants() only appends lines matching ^ Name {$. A concrete future declaration such as HealthCheck, or LegacyRequest(u8), inside ClientRequestV1 is therefore silently omitted. With the matrix left unchanged, both parsed lists remain identical and main() prints success and returns 0, even though the new enum variant has no matrix row. The later set/order checks cannot catch a variant that the parser never collected, and there is no independent variant-count assertion. This behavior is introduced by the new checker and contradicts the PR's stated contract that future/current variants are automatically included in the exhaustive comparison.

Root cause: the enum parser treats only struct-style rust variants as variants instead of parsing every top-level clientrequestv1 variant or failing closed on unsupported variant syntax. consequently, valid unit and tuple variants bypass the security-sensitive gate-matrix completeness check.

[major] 104-variant baseline ignores the TypeBox source of truth

docs/plan-refactor-c2s-gate-v1.md:14 · schema-contracts

The new plan declares server/src/schema/client_request.rs::ClientRequestV1 to be the authoritative 104-variant set and marks P0 complete based on scripts/check_c2s_gate_matrix.py, but that checker only compares the Rust mirror with the Markdown matrix. Bong's IPC source of truth is the TypeBox schema, and its actual ClientRequestV1 union in agent/packages/schema/src/client-request.ts:1135-1223 does not contain multiple Rust/matrix variants, including AlchemyLearnRecipeFragment, CoffinBreak, CoffinMenuReclaim, QiScatterBeadUse, SupplyCoffinOpen, ExternalContainerMove, CraftStart, and GiveDanToElder. Therefore the advertised checker can report that all 104 requests match while the authoritative schema and generated artifacts expose a different request set. No surrounding check in this script reads TypeBox or generated JSON Schema, so the mismatch is not prevented.

Root cause: the plan reverses the repository's schema authority by treating the rust serde enum as canonical, and the new validation checks only that mirror against documentation instead of validating typebox, generated json schema, rust, and consumers together.

Central review

Decision: request_changes
Reviewed head: 353c013b79dae604e2e075d4b8772dc27b0784fc
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Add negative contract tests for the four-way gate-matrix checker

scripts/tests/check_c2s_gate_matrix_test.py:14 · testing

The newly wired CI step runs the checker only against the repository's currently consistent files, while ParserTests contains only three parse_enum_variants tests. There is no negative end-to-end validation for the checker's claimed contract: mismatched sets, reordered variants, duplicate variants, non-contiguous matrix rows, missing TypeBox literals, or generated-schema drift. A concrete regression such as deleting the comparisons at scripts/check_c2s_gate_matrix.py:162-194 (or making main() return success after parsing) would still pass every unittest and would also pass the workflow's positive-only invocation at .github/workflows/e2e.yml:121, so CI would stop rejecting schema/matrix drift without any test failure.

Root cause: the test suite exercises only the rust enum parser and never drives the four-representation validation logic with deliberately inconsistent fixtures. the positive repository check proves only that today's inputs agree; it does not regression-protect the checker's failure behavior, which is the observable contract this pr adds to ci.

[major] Craft quantity schema omits the server's maximum of 64

agent/packages/schema/src/client-request.ts:1316 · schema-contracts

CraftStartRequestV1.quantity is constrained only with minimum: 1, so the TypeBox source and generated JSON Schema accept every positive JavaScript integer, including 65. The Rust mirror independently enforces MAX_CRAFT_QUANTITY_V1, which resolves to crate::craft::MAX_CRAFT_QUANTITY = 64, in deserialize_craft_quantity (server/src/schema/client_request.rs:735-748). Thus a payload such as {"type":"craft_start","v":1,"recipe_id":"x","quantity":65} validates against the IPC source of truth but is rejected by the server decoder. The new matrix/checker only compares discriminator sets/order and cannot detect this field-constraint drift.

Root cause: the newly added typebox contract omitted the server's canonical maximum craft quantity, leaving the producer/schema boundary broader than the rust serde consumer contract.

[major] Pin the new craft quantity boundary in schema tests

agent/packages/schema/src/client-request.ts:1316 · testing

CraftStartRequestV1 introduces the observable contract that quantity must be an integer >= 1, but the schema test suite contains no craft_start positive or boundary/negative pin. The new matrix checker cannot protect this constraint: typebox_wire_literals() extracts only each type literal and main() compares variant names/order, so changing this line to Type.Integer() (and regenerating the JSON schema) would still pass both the checker and its parser-only unit tests while accepting quantity: 0 and negative quantities. Existing ClientRequestV1 tests cover other request families but do not mention craft_start, so no surrounding test rejects that concrete regression.

Root cause: the pr adds request payload validation contracts but adds only union-membership/order checking, not dedicated schema behavior tests for the new fields and their boundaries. consequently, field-level constraints can regress consistently across source and generated artifacts without any test failing.

[major] zhenfa_place permits remote cross-dimension world mutation

agent/packages/schema/generated/client-request-v1.json:1452 · security

An authenticated client can now send the newly published {"v":1,"type":"zhenfa_place","x":...,"y":...,"z":...,"kind":...,"qi_invest_ratio":...} contract with arbitrary integer coordinates (the changed schema has no proximity or dimension field/constraint). The production dispatcher forwards those coordinates directly as ZhenfaPlaceRequest.pos (server/src/network/client_request_handler.rs:1630-1658) without resolving an authoritative target or checking the player's position/dimension. The consumer then checks inventory/realm/qi but never queries player position or dimension before place_zhenfa_anchor_block mutates the Overworld layer at the supplied position (server/src/zhenfa/mod.rs:1573-1782). Thus a player with the required item/resources can place array/trap blocks at arbitrary remote Overworld coordinates, including while located in another dimension. TypeBox/JSON validation does not prevent this because all integers are accepted, and the later domain checks do not add a spatial authority check. This PR exposes the request through the generated client schema, making the unsafe server-supported operation part of the distributed C2S contract.

Root cause: the generated trust-boundary contract publishes zhenfa_place with attacker-controlled world coordinates while the real production handler treats those coordinates as authoritative and performs mutation without a server-side same-dimension/reach gate.

[major] Matrix-only changes bypass the new C2S contract gate

.github/workflows/e2e.yml:119 · wiring

The new contract step runs only inside the e2e workflow, but the workflow's pull_request.paths filter (lines 16-23) includes agent, client, server, proto, scripts, compose, and the workflow itself, not docs/**. The checker reads docs/plan-refactor-c2s-gate-v1.md as one of its four authoritative inputs. A pull request that changes only that matrix can therefore add, remove, reorder, or rename a row without starting this workflow, so the newly added step at line 119 never validates the changed contract. No other workflow references check_c2s_gate_matrix.py.

Root cause: the checker is registered in a path-filtered workflow without adding its markdown matrix input to the workflow trigger paths, leaving one producer of the validated contract disconnected from ci.

[major] TypeBox union parser silently skips valid unrecognized members

scripts/check_c2s_gate_matrix.py:91 · schema-contracts

typebox_union_members() builds the authoritative union member list by retaining only lines that match a bare Identifier, and silently ignores every other non-empty line in the Type.Union block. For example, adding a valid inline member such as Type.Object({ v: Type.Literal(1), type: Type.Literal("new_request") }), while leaving the generated artifact stale causes that entire source member to be omitted; the collected members can still equal the unchanged Rust enum and generated JSON Schema, so main() exits successfully. The new parser tests exercise unsupported Rust syntax only and do not cover this TypeBox fail-open path. This defeats the PR's stated exhaustive TypeBox-source-to-Rust/matrix/generated-schema contract and allows the source-of-truth schema to drift without the CI gate detecting it.

Root cause: the typebox union parser treats unmatched syntax as ignorable whitespace instead of rejecting any non-comment, non-member content inside the authoritative union. consequently, valid typebox member expressions outside the parser's narrow bare-identifier form disappear from validation rather than failing closed.

[major] Trigger the matrix check when its documentation input changes

scripts/check_c2s_gate_matrix.py:12 · wiring

The checker makes docs/plan-refactor-c2s-gate-v1.md one of its four authoritative inputs via PLAN_PATH, but the e2e workflow's push and pull_request path filters include agent/**, client/**, server/**, proto/**, scripts/**, and the workflow itself, not docs/** (.github/workflows/e2e.yml:3-22). A PR that adds, removes, renumbers, or reorders only matrix rows therefore never starts the job containing python3 scripts/check_c2s_gate_matrix.py. The checker cannot catch that drift, despite the new plan explicitly claiming CI enforces the four-way contract. This is exposed by this PR because it newly makes a documentation file a required machine-checked contract input.

Root cause: the ci registration for the new checker is not triggered by changes to every file the checker consumes; specifically, its documentation matrix input is outside the workflow path filters.

[major] New C2S request constraints have no semantic regression tests

agent/packages/schema/generated/client-request-v1.json:4019 · testing

The PR adds new request-shape constraints, including craft_start.quantity with minimum: 1 here, plus complex external_container_move locations and bounded IDs, but the schema tests contain no references to craft_start, craft_cancel, external_container_move, external_container_close, lingtian_start_till, lingtian_start_renew, lingtian_start_planting, lingtian_start_harvest, lingtian_start_drain_qi, qi_scatter_bead_use, alchemy_learn_recipe_fragment, coffin_break, coffin_menu_reclaim, supply_coffin_open, container_open, workbench_open, or give_dan_to_elder. The generated-artifact freshness test only checks that committed JSON matches the TypeBox source, and the new C2S matrix checker only compares discriminator membership/order. Consequently, changing the TypeBox source to accept {v:1,type:"craft_start",recipe_id:"r",quantity:0} and regenerating this file would still satisfy those tests even though it violates the newly changed minimum-one contract; analogous shape regressions in the other new variants are also unpinned.

Root cause: the pr adds and regenerates many observable clientrequestv1 contracts without dedicated positive and negative schema tests for their fields, boundaries, required properties, and closed-object behavior. existing parity/freshness checks prove representation synchronization, not the semantic constraints of the new request variants.

[major] Make qi-scatter coordinates atomic in the wire schema

agent/packages/schema/generated/client-request-v1.json:1673 · strict-maintainability

The new generated qi_scatter_bead_use schema requires only item_instance_id; x, y, and z are independently optional at lines 1673-1681. Therefore a payload such as {"type":"qi_scatter_bead_use","v":1,"item_instance_id":7002,"x":1} is schema-valid. The real server consumer in server/src/network/client_request_handler.rs:1700-1708 accepts coordinates only when all three are present or all three are absent and otherwise drops the request without dispatching ScatterBeadUseRequest. Existing tests cover only all-present and all-absent payloads, so they do not prevent this producer/consumer mismatch. This contract is newly exposed by adding this request variant to the generated schema in the pull request.

Root cause: a logically atomic optional coordinate tuple was modeled as three independent optional properties, while the production handler enforces an all-or-none invariant that the schema does not express. as a result, schema-valid client requests can be silently rejected by the server.

[major] Invalid Lingtian modes are accepted and executed as manual

agent/packages/schema/src/client-request.ts:1245 · correctness

Both newly added Lingtian request schemas accept any string for mode. A payload such as {"type":"lingtian_start_harvest","v":1,"x":0,"y":64,"z":0,"mode":"invalid"} therefore passes the TypeBox contract. The production handler forwards parse_session_mode(&mode) at server/src/network/client_request_handler.rs:2584-2587, and that parser maps every value other than case-insensitive auto to SessionMode::Manual (server/src/network/client_request_handler.rs:13752-13756). The same conversion is used for till requests at lines 2540-2545. Consequently malformed but schema-valid modes start a manual gameplay session instead of being rejected; existing validation does not prevent this because the schema is open-ended and the parser deliberately defaults.

Root cause: the typebox source represents the closed manual/auto state as an unrestricted string while the server parser converts unknown values to a real state rather than rejecting them.

[major] Schema-valid partial bead coordinates are dropped by the server

agent/packages/schema/src/client-request.ts:942 · wiring

QiScatterBeadUseRequestV1 declares x, y, and z as three independently optional fields, so the TypeBox source of truth and generated schema accept payloads such as {"v":1,"type":"qi_scatter_bead_use","item_instance_id":1,"x":10}. The real server ingress deserializes that payload, but client_request_handler.rs:1700-1708 only accepts all three coordinates or none and drops every partial tuple as malformed before emitting ScatterBeadUseRequest. No TypeBox validation prevents this because each coordinate is individually optional. This mismatch is introduced by the newly added union member and makes a schema-valid request unreachable through the production consumer.

Root cause: the producer-side schema models a logically atomic optional coordinate tuple as three unrelated optional properties, while the server consumer enforces all-or-none tuple presence.

[major] Rust serde wire renames bypass the new matrix contract

scripts/check_c2s_gate_matrix.py:169 · strict-maintainability

main() constructs the Rust wire list as [snake_case(variant) for variant in enum] instead of deriving it from the enum's serde contract. For example, changing ClientRequestV1's existing #[serde(..., rename_all = "snake_case")] to rename_all = "camelCase" (or adding a variant-level #[serde(rename = "...")]) leaves the parsed variant names unchanged, so this checker can still report success while Rust deserializes a different discriminator from TypeBox and the generated JSON Schema. parse_enum_variants() explicitly skips every #[...] line, and the new tests only exercise variant shapes; they do not pin enum- or variant-level serde naming. Thus the new CI step does not enforce the claimed Rust-wire side of the four-way contract and accepts a concrete cross-stack schema break introduced after this PR.

Root cause: the checker treats a locally implemented pascalcase-to-snake_case conversion as the authoritative rust wire representation and discards the serde attributes that actually define that representation.

[major] Generated craft_start contract omits the server's quantity cap

agent/packages/schema/generated/client-request-v1.json:4018 · wiring

The changed craft_start.quantity schema only sets minimum: 1, so payload {"type":"craft_start","v":1,"recipe_id":"craft.example.herb_knife.iron","quantity":65} validates against this generated ClientRequestV1 artifact. The real Rust ingress rejects that same payload: server/src/schema/client_request.rs:734-748 binds the maximum to crate::craft::MAX_CRAFT_QUANTITY, which is 64 (server/src/craft/session.rs:34), and server/src/schema/client_request.rs:2407-2408 explicitly pins 65 as invalid. The separate craft contract also declares and tests the 64 cap (agent/packages/schema/src/craft.ts:21,115; agent/packages/schema/tests/craft.test.ts:54-62). Thus schema-driven producers or validators can accept a request that the production consumer drops during deserialization; none of the surrounding generated constraints supplies the missing upper bound.

Root cause: the clientrequestv1 craft-start representation was generated from craftstartrequestv1, whose quantity field carries only a minimum, instead of reusing the canonical craft quantity constraint with maximum 64. this leaves the generated producer/validator contract out of sync with the rust wire consumer and the existing craft schema.

[major] Add contract tests for the new NPC request schemas

agent/packages/schema/generated/client-request-v1.json:1349 · testing

The generated union adds the npc_inspect_request, npc_dialogue_choice, and npc_trade_request contracts here, including required fields and bounds such as npc_entity_id >= 0, nonblank/length-bounded option_id, safe item IDs, and nonblank/length-bounded requested_item_id (lines 1343-1432). A repository-wide search of schema tests finds no references to any of these three discriminator values. The newly added checker also cannot protect these constraints: generated_typebox_wires() reads only variant["properties"]["type"]["const"] (scripts/check_c2s_gate_matrix.py:111-116), and its unit tests cover only Rust enum parsing (scripts/tests/check_c2s_gate_matrix_test.py:14-46). Consequently, a concrete regression such as replacing npc_entity_id: Type.Integer({ minimum: 0 }) with an unbounded integer, dropping the nonblank pattern, or making one of these fields optional in the TypeBox source and regenerating this file would still satisfy the matrix checker and all current tests, while malformed NPC requests would become valid on the wire.

Root cause: the pr adds observable validation contracts for the npc request variants but provides no positive, boundary, or negative validation tests for them; the only new automated gate verifies discriminator membership and ordering rather than request payload shape.

[major] Failed external-container moves can permanently drop the player's item

agent/packages/schema/generated/client-request-v1.json:3208 · concurrency-atomicity

The newly generated external_container_move contract requires an attacker-controlled from location and explicitly accepts container, equip, or hotbar locations (lines 3208 and 3230 onward). On the reachable player-to-external-container path, handle_external_container_move does not verify that instance_id is at from: it scans every player container and removes the first matching item (server/src/network/client_request_handler.rs:18497-18506). If placing into the external container then fails, for example because the requested target cells are occupied, rollback only searches for the declared from when it is a container (18564-18577). Sending an item that is actually in main_pack with from:{"kind":"hotbar","index":0} and an occupied external target therefore removes the real item, fails placement, finds no rollback container, and returns after resync with the item absent from both inventories. Schema validation does not prevent this because the forged location is structurally valid, and the existing tests exercise valid container origins rather than this rollback case.

Root cause: the new move request exposes a non-atomic remove-then-place implementation whose rollback trusts the unverified client-supplied from location instead of retaining and restoring the actual source container/location from which the item was removed.

Central review

Decision: infrastructure_failure
Reviewed head: 46c8406fdba53db7e75f2c45b57fdf3c0bfc7ee1
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

The review could not complete safely. No approval or code finding was inferred from the failed stages.

• find:wiring — infra_error: 2 occurrence(s) in 2/5 failed batch(es); samples: batch-1: after 3 attempts: timeout after 300000ms | batch-2: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"}],"stderr":""} {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":524,"terminalReason":"api_error"}],"stderr":""}
• find:strict-maintainability — infra_error: 1 occurrence(s) in 1/5 failed batch(es); samples: batch-2: after 3 attempts: timeout after 300000ms
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"}],"stderr":""}

Central review

Decision: infrastructure_failure
Reviewed head: 46c8406fdba53db7e75f2c45b57fdf3c0bfc7ee1
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

The review could not complete safely. No approval or code finding was inferred from the failed stages.

• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:0:attempt-1 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:1:attempt-2 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:2:attempt-3 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:3:attempt-4 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:4:attempt-5 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:0:attempt-6 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:1:attempt-7 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:2:attempt-8 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:3:attempt-9 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:4:attempt-10 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:0:attempt-11 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:1:attempt-12 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:2:attempt-13 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:3:attempt-14 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2:4:attempt-15 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f0ddeac80f5b7b092cde9b87de974bebb909a4bd8fc902cb23a9f91548dbe9ad:2 — infra_error: could not collect 5 valid votes after 15 attempts
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:0:attempt-1 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:1:attempt-2 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:2:attempt-3 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:3:attempt-4 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:4:attempt-5 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:0:attempt-6 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:1:attempt-7 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:2:attempt-8 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:3:attempt-9 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:4:attempt-10 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:0:attempt-11 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:1:attempt-12 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:2:attempt-13 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:3:attempt-14 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1:4:attempt-15 — infra_error: after 3 attempts: claude exited 1
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
• validate:f6b47ce51871f40a32950fbb126f7a94b4c3b82b778c5970beb0a8a6d23199aa:1 — infra_error: could not collect 5 valid votes after 15 attempts

Central review

Decision: request_changes
Reviewed head: 46c8406fdba53db7e75f2c45b57fdf3c0bfc7ee1
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Lingtian replenish required and closed-shape contract is omitted

agent/packages/schema/tests/client-request-new-contracts.test.ts:43 · testing

lingtian_start_replenish is the only newly added Lingtian request omitted from the requests table, so this suite never verifies its required x, y, z, and source fields or its closed top-level shape. The pre-existing tests in agent/packages/schema/tests/schema.test.ts:1802 and :1814 only validate one accepted source and one rejected source. An incorrect implementation that made a coordinate optional or allowed additional properties would therefore pass all current tests, despite the new request contract requiring all four fields and additionalProperties: false.

Root cause: the exhaustive-looking new-contract table skips one newly added union variant, and existing source-enum tests do not cover that variant's required-field or closed-object guarantees.

[major] Do not deduplicate generated schema wires before validating duplicates

scripts/check_c2s_gate_matrix.py:148 · schema-contracts

generated_typebox_wires() recursively extracts every generated type.const, then immediately returns list(dict.fromkeys(wires(document))) at scripts/check_c2s_gate_matrix.py:148. Consequently, a generated schema whose anyOf contains duplicate wire variants, for example alpha, alpha, beta, is reduced to alpha, beta before main() calls duplicates(generated). The checker therefore reports success whenever the deduplicated sequence matches TypeBox, despite the generated JSON Schema containing a duplicate union member.

Root cause: the generated-schema representation is deduplicated before validation, which destroys the evidence needed by the duplicate-contract check and allows malformed or drifted generated unions to pass the ci gate.

[major] Recipe-fragment request has no production producer

agent/packages/schema/src/client-request.ts:545 · wiring

AlchemyLearnRecipeFragmentRequestV1 is added to the authoritative ClientRequestV1 union with the wire discriminator alchemy_learn_recipe_fragment and an item_instance_id, and the Rust handler consumes that discriminator by emitting LearnRecipeFragmentIntent (server/src/network/client_request_handler.rs:884-896). However, there is no production client or agent producer for this discriminator: repository-wide client/agent matches find it only in the TypeBox declaration and schema tests. The actual alchemy fragment drop path still calls sendAlchemyLearnRecipe(id) (client/src/main/java/com/bong/client/alchemy/AlchemyScreen.java:724-732), whose sender emits the older alchemy_learn_recipe payload with a recipe string (client/src/main/java/com/bong/client/network/ClientRequestSender.java:428-430). Therefore dragging a recipe fragment never traverses the newly added instance-ID request path, so the server-side fragment consumer cannot be reached through the real UI; schema tests only inject the missing payload directly.

Root cause: the pr registers a new cross-stack request contract and server consumer but does not wire the existing alchemy fragment ui action to a matching client encoder/sender. the production producer remains connected to the legacy recipe-id request, leaving alchemy_learn_recipe_fragment disconnected outside tests.

[major] Pin entity IDs to the Rust i32 wire range

agent/packages/schema/tests/client-request-new-contracts.test.ts:15 · correctness

The new contract table supplies ordinary positive entity_id values for supply_coffin_open, container_open, and workbench_open, and the generic assertions only remove required fields or add an unknown property. Their new TypeBox schemas use unconstrained Type.Integer(), while the Rust mirror stores each field as i32 (server/src/schema/client_request.rs:520-537). Consequently a payload such as {v:1,type:"workbench_open",entity_id:2147483648} passes the TypeBox source-of-truth validator but cannot deserialize into the Rust request. Nothing in this test catches either signed-32-bit boundary, so the changed cross-stack contract remains green despite rejecting a schema-valid request at the server boundary.

Root cause: the new entity-id contracts were tested only for presence and closed shape, without pinning the numeric range shared by typebox and rust. the typebox producer contract is therefore wider than the rust i32 consumer contract.

[major] Reconcile the P0-only claim with runtime changes in this PR

docs/plan-refactor-c2s-gate-v1.md:21 · correctness

This plan says the PR “只完成 P0 docs closeout” and that runtime implementation remains blocked, and later repeats that P0 does not modify the runtime gate. However, this pull request also changes the production handler: server/src/network/client_request_handler.rs:1642-1657 now applies an Overworld/6-block Zhenfa placement gate, and the same changed file alters Lingtian mode rejection and external-container mutation handling. The matrix still labels ZhenfaPlace as “空间门缺” at line 177. Thus the document's phase status, scope, and current-state matrix do not describe the code being merged, and the selected runtime fixes bypass the plan's stated P1 prerequisite and atomic full-gate rollout.

Root cause: the plan was written against an earlier origin/main snapshot and marked p0 complete without reconciling concurrent production changes in the same pull request, leaving its claimed deliverables and implementation status stale.

[major] Cover Zhenfa placement gate through the client-request path

server/src/network/client_request_handler.rs:1642 · testing

The handler now rejects every zhenfa_place request unless the requester has position and dimension state, is in Overworld, and is within squared distance 36 of the block center (server/src/network/client_request_handler.rs:1642-1655). The repository-visible tests cover schema/protobuf round trips and Zhenfa event handling, but no test exercises a real client_request payload through this new handler branch with an accepted boundary case and rejected cross-dimension, non-Overworld, missing-component, and out-of-range cases. A concrete incorrect implementation that dispatches the request without this gate would therefore pass the current tests despite violating the changed authorization contract.

Root cause: a new security-sensitive c2s reach/dimension gate lacks production-path regression coverage. tests that deserialize or directly inject zhenfaplacerequest cannot detect omission or misordering of the handler-side gate.

[major] New request boundary constraints are not regression-pinned

agent/packages/schema/tests/client-request-new-contracts.test.ts:66 · testing

The new parameterized suite supplies only small valid values for every bounded identifier (item_instance_id, session_id, instance_id, hoe_instance_id, and pill_instance_id) and checks only required-field removal plus an extra top-level property. The later boundary test covers craft_start.quantity, modes, and one scatter-coordinate partial case, while the NPC test only checks the lower bound for npc_entity_id and whitespace strings. Consequently, concrete incorrect schema changes such as removing the nonnegative/JavaScript-safe upper bounds from the newly added instance/session IDs, or removing the 64/128-character upper bounds from NPC strings, would still pass this suite. No surrounding tests found cover those bounds for these new contracts. These constraints are observable IPC validation behavior introduced by this PR, and the project policy requires positive, maximum, and off-by-one negative pins for changed schema contracts.

Root cause: the shared test table validates presence and closed shapes but does not encode the numeric and string boundary constraints of the new request schemas, leaving multiple changed wire contracts unprotected against permissive regressions.

[major] Recipe-fragment learning has no production client producer

agent/packages/schema/generated/client-request-v1.json:722 · wiring

The changed generated union exposes alchemy_learn_recipe_fragment as a production C2S request requiring item_instance_id at lines 708-730, and the server has a real dispatch branch in server/src/network/client_request_handler.rs:884-896. However, a repository-wide search for alchemy_learn_recipe_fragment finds only the TypeBox declaration, schema tests, and server-side schema/handler tests; there is no client ClientRequestProtocol encoder, ClientRequestSender method, or UI/item-use handler that emits it. The existing alchemy screen only emits alchemy_learn_recipe (client/src/main/java/com/bong/client/alchemy/AlchemyScreen.java:731), while recipe-fragment client code in InventorySnapshotHandler.java:381,421 only renders fragment metadata. Current tests manually construct JSON or deserialize fixtures, so they do not exercise a production client-to-server path.

Root cause: the pr registers the new request in the shared schema and wires its server consumer, but omits the client-side producer and interaction entry point. consequently a normal client can never send the request, leaving recipe-fragment learning unreachable despite schema and synthetic server tests passing.

[major] Rust still accepts craft_start payloads rejected by the source schema

agent/packages/schema/src/client-request.ts:1345 · schema-contracts

A {"v":1,"type":"craft_start","recipe_id":"r"} request fails the changed TypeBox/generated contract because quantity is required here, but the Rust mirror still marks quantity with #[serde(default = "default_craft_quantity", ...)] (server/src/schema/client_request.rs:664-668), and its regression test explicitly expects the omitted field to deserialize as quantity 1 (server/src/schema/client_request.rs:2397-2401). The production handler then dispatches the resulting CraftStartIntent (server/src/network/client_request_handler.rs:2781-2796). Thus a client that talks directly to the server can submit a payload the IPC source of truth declares invalid and still start a craft; TypeBox tests cannot prevent this because they only exercise the agent schema.

Root cause: the pull request adds craft_start to the authoritative typebox union with mandatory quantity, but does not align the rust serde mirror, which preserves an omitted-field compatibility default. validation therefore differs across the typebox-to-rust boundary.

[major] New C2S contracts lack required boundary and per-variant regression tests

agent/packages/schema/src/client-request.ts:1228 · testing

The new schemas introduce bounded IDs such as ExternalContainerMoveRequestV1.session_id/instance_id (0..JS_SAFE_INTEGER_MAX) and several other newly bounded item IDs, but client-request-new-contracts.test.ts supplies only the interior value 1 and checks required fields/unknown top-level fields. It never checks 0, JS_SAFE_INTEGER_MAX, -1, or JS_SAFE_INTEGER_MAX + 1 for these new contracts. Consequently, concrete incorrect changes such as changing minimum to 1 or deleting maximum would leave the entire current TypeBox test suite green while changing which C2S payloads are accepted. The same suite also omits the newly added jiemai discriminator entirely, so an incorrect required field or an open object shape for that parameterless request would pass; the static matrix checker only compares discriminator membership/order and cannot detect field-constraint drift.

Root cause: the regression suite exercises representative happy-path values but does not pin the newly declared numeric boundaries or every new request contract. this leaves observable c2s validation behavior unprotected, contrary to the required saturated schema tests for boundary, off-by-one, invalid-input, and per-variant coverage.

[major] Test strict Lingtian mode rejection and dispatch

server/src/network/client_request_handler.rs:2556 · testing

Unknown Lingtian till and harvest modes are now required to be rejected before StartTillRequest/StartHarvestRequest is sent (server/src/network/client_request_handler.rs:2556-2569 and 2606-2619). The changed handler has no corresponding tests for manual, auto, case-insensitive forms, and invalid modes proving both event emission and no-op rejection. A regression that restores the previous fallback of treating every unknown mode as manual would pass the existing visible tests while violating the new request contract.

Root cause: the strict mode-parsing behavior changes an externally supplied request value from fail-open defaulting to rejection, but the event-dispatch boundary and invalid-input branch are unprotected by regression tests.

Central review

Decision: request_changes
Reviewed head: a83703389412707cde0e744affce01bf303bea2f
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Stacked attributes bypass the variant wire-rename guard

scripts/check_c2s_gate_matrix.py:53 · schema-contracts

A future valid Rust variant declaration such as #[serde(rename = "renamed")] followed by #[cfg(feature = "x")] and then Variant, reaches this parser. Line 53 overwrites pending_attribute for every attribute, so by the time lines 56-57 inspect it, only the cfg attribute remains and the unsupported serde rename is accepted. The matrix is then compared against the Rust identifier Variant, while the actual wire discriminator is renamed, and main() exits successfully. Rust/Serde permits stacked attributes, no surrounding validation rejects this shape, and the added test covers only a serde rename immediately adjacent to the variant, so this incorrect implementation still passes the new suite.

Root cause: the parser stores only the most recent top-level variant attribute instead of accumulating and validating all attributes attached to the pending variant. this makes the new purported fail-closed wire-contract check order-dependent and allows a serde rename to be hidden by any following attribute.

[major] Inline-attributed variants are silently omitted from the gate matrix inventory

scripts/check_c2s_gate_matrix.py:52 · strict-maintainability

parse_enum_variants treats every depth-1 line whose stripped text starts with # as an attribute-only line and immediately continues. Rust permits an outer attribute and its variant on the same line, for example #[cfg(feature = "x")] NewRequest { v: u8 }, or #[serde(rename = "new_request")] NewRequest,. For that valid declaration, the checker never appends NewRequest; if the Markdown matrix also omits it, the set/order checks in main() pass even though ClientRequestV1 gained a variant. The new tests only cover attributes on their own line, so they do not expose this accepted omission. This defeats the changed plan's stated contract that future variants are automatically included and that unknown top-level syntax fails closed.

Root cause: the parser identifies an attribute solely by code.startswith("#") and neither parses nor rejects tokens following the closing attribute bracket, allowing a complete variant declaration on the same line to be silently discarded from the canonical enum inventory.

[minor] Plan edits unnecessarily trigger the full 45-minute E2E job

.github/workflows/e2e.yml:12 · performance

A documentation-only commit that changes docs/plan-refactor-c2s-gate-v1.md now matches the push and pull_request path filters at .github/workflows/e2e.yml:12 (and the corresponding PR filter). That launches the entire E2E Redis Smoke job, including Java/Gradle client tests, Node dependency installation and schema/agent tests, Rust release build and server tests, Redis setup, smoke E2E, and bot scenarios. The newly added matrix checker itself is a small static check and does not require this full environment, so each plan edit consumes a full GitHub-hosted runner and can add up to the job's 45-minute timeout to documentation iteration.

Root cause: the workflow path filter couples a planning-document change to the full end-to-end job instead of a focused matrix-contract job. because the workflow's existing job has broad build, integration, and bot stages, ordinary edits to this large plan cause substantial avoidable ci latency and runner consumption.

[major] Stacked attributes bypass the tested serde-rename rejection

scripts/tests/check_c2s_gate_matrix_test.py:53 · testing

The new negative test only covers a #[serde(rename = "other")] attribute immediately adjacent to the variant. A valid declaration such as #[serde(rename = "other")]\n #[doc = "variant docs"]\n Variant, is accepted by parse_enum_variants: each top-level attribute overwrites pending_attribute, so by the time Variant is parsed only the doc attribute remains and the serde rename is never rejected. main() then compares the Rust identifier Variant with the Markdown row and returns success even though the actual wire discriminator is other. The workflow's direct checker invocation does not compensate because it uses the same faulty parser. This contradicts the stated P0 contract that variant-level wire renames fail closed, and the current suite would pass this concrete incorrect implementation.

Root cause: the parser regression tests cover only one immediately adjacent attribute and omit valid stacked variant attributes. consequently, the checker's single-slot pending_attribute state can discard a previously seen serde rename, allowing wire-schema drift through the newly added ci contract.

Central review

Decision: infrastructure_failure
Reviewed head: 7a55c8c0fc74985a9696121818ab58db52267626
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

The review could not complete safely. No approval or code finding was inferred from the failed stages.

• find:correctness — infra_error: 1 occurrence(s) in 1/2 failed batch(es); samples: batch-0: after 3 attempts: stdout limit exceeded (1000000 bytes)
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"non_json_output"}],"stderr":""}

Central review

Decision: request_changes
Reviewed head: 7a55c8c0fc74985a9696121818ab58db52267626
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Remove unrelated item textures from the P0 gate-plan closeout

client/src/main/resources/assets/bong-client/textures/gui/items/herb_knife_iron.png:1 · strict-maintainability

This PR's declared P0 deliverable says it "only adds docs, absorption evidence, and the enum/matrix static checker" (docs/plan-refactor-c2s-gate-v1.md:297-300), and the implementation-workflow section explicitly says this plan produces no textures (docs/plan-refactor-c2s-gate-v1.md:334). Nevertheless, it also adds four production client item textures: this file plus rabbit_pelt.png, yu_yi_zhi.png, and zhenfa_trap_iron.png. These assets are unrelated to the C2S gate matrix contract and are not mentioned, validated, or owned by any P0 acceptance item. They therefore expand the shipped client/resource surface in a plan-closeout PR without an explicit migration or current plan requirement.

Root cause: unrelated gameplay assets were bundled into a docs-and-static-checker phase even though the changed plan explicitly excludes texture output and defines a narrower p0 file/deliverable boundary. this obscures the authoritative purpose and validation surface of the change; the assets should be moved to the gameplay/asset change that owns and verifies them, or the plan and acceptance evidence must explicitly absorb that scope.

[major] Reject trailing variant declarations instead of silently skipping them

scripts/check_c2s_gate_matrix.py:98 · correctness

A valid Rust declaration can place more than one variant on a line, for example pub enum ClientRequestV1 {\n Alpha, Beta,\n}. At line 98 the suffix check accepts this line because the suffix starts with ,, but line 100 appends only the first regex capture (Alpha) and never verifies that the remainder contains no second declaration. With a matrix containing only Alpha, parse_enum_variants() returns ["Alpha"] and main() reports success even though the production enum also contains Beta. The same bypass applies to Alpha { v: u8 }, Beta, and tuple forms. Rustfmt would normally split these, but this checker runs as its own CI gate and the parser explicitly promises to fail closed on unsupported top-level enum syntax; no surrounding validation in this script detects the skipped variant, and the added tests cover unknown discriminant syntax but not multiple valid declarations on one line.

Root cause: the line-oriented parser validates only the first character of the text following the first variant name and then discards the rest of that line. it neither consumes one complete rust variant declaration nor rejects trailing non-comment tokens, so additional valid variants can be silently omitted from the inventory and therefore from the required gate matrix.

[major] Plan matrix parsing is mocked out of every regression test

scripts/tests/check_c2s_gate_matrix_test.py:105 · testing

The new test suite never exercises matrix_variants() against plan text or a temporary plan file. Instead, both the failure matrix and success test patch matrix_variants (lines 105 and 113) to return already-parsed values. A concrete incorrect implementation such as returning the current 104-row baseline unconditionally would therefore pass every unittest and the workflow's live checker invocation while the repository is initially in sync; after a plan row is renamed, omitted, duplicated, or malformed, that implementation would continue reporting success. The tests of main() only prove validation after parsing and cannot expose this broken document-to-row boundary.

Root cause: the plan-side parser, which is a changed observable contract of the new ci gate, has no direct regression test; all contract tests mock away that boundary. add parser tests using representative markdown for valid rows and malformed, missing, duplicate, reordered, and unrelated numbered tables, or an end-to-end test with controlled enum and plan files.

Central review

Decision: infrastructure_failure
Reviewed head: 1d042f726787a9aaef722f85df2c2c0302c39df4
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

The review could not complete safely. No approval or code finding was inferred from the failed stages.

• find:correctness — schema_error: 1 occurrence(s) in 1/2 failed batch(es); samples: batch-0: schema validation failed: candidate-0: candidate taxonomy must exactly equal assigned dimension "correctness"

Central review

Decision: request_changes
Reviewed head: 1d042f726787a9aaef722f85df2c2c0302c39df4
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] Matrix-only changes bypass the new CI contract gate

.github/workflows/e2e.yml:119 · correctness

The new contract step at lines 119-122 reads docs/plan-refactor-c2s-gate-v1.md, but the workflow's pull_request.paths list at lines 15-22 includes no docs/** path. A pull request that edits, reorders, duplicates, or removes a matrix row without touching server/scripts/workflow files therefore does not start this workflow at all, so check_c2s_gate_matrix.py cannot reject the drift. This directly contradicts the plan's changed claim that CI forces the Rust-enum/Markdown comparison.

Root cause: the checker was registered as a workflow step, but one of its authoritative input paths was omitted from the workflow trigger filters, leaving matrix-only changes outside the claimed contract gate.

[major] AlchemyOpenFurnace matrix drops the existing owner gate

docs/plan-refactor-c2s-gate-v1.md:148 · correctness

Matrix row 11 assigns AlchemyOpenFurnace no ownership/participant gate (—), even though the real production path calls with_owned_furnace_mut (server/src/network/client_request_handler.rs:16302), which rejects a non-owner for an owned furnace, and the absorbed alchemy plan explicitly requires preserving the existing owner-exclusive behavior (docs/plans-skeleton/plan-bughunt-alchemy-furnace-scope-gate.md:64). The new plan says this ownership column is the P3 terminal contract and P3 will delete duplicate domain owner checks. Implementing the row as written and then removing the existing check makes an in-range player able to open another player's furnace and receive its snapshots; the generic distance/dimension/state gates do not prevent that.

Root cause: the exhaustive gate matrix fails to carry the existing owned-or-public furnace authorization predicate for alchemyopenfurnace, despite making the matrix authoritative for the later removal of domain checks.

[major] Serde contract check can be spoofed by a decoy source match

scripts/check_c2s_gate_matrix.py:12 · security

ENUM_SERDE_RE is an unanchored search over the entire Rust source (scripts/check_c2s_gate_matrix.py:11-12). A contributor can place a comment or string containing #[serde(tag = "type", rename_all = "snake_case")] pub enum ClientRequestV1 { before the real declaration, then change the real enum's serde tag or casing. parse_enum_variants accepts the decoy match's attributes at line 57-61, while the later exact-line scan still extracts variants from the real enum, so the checker can report a matching matrix even though the production enum no longer has the required wire contract. This is reachable from a pull request modifying server/src/schema/client_request.rs and bypasses the CI artifact/integrity check that is supposed to fail closed on serde drift.

Root cause: the serde contract is validated against the first textual regex match rather than the actual pub enum clientrequestv1 declaration. comments and string literals are not excluded, allowing source text to spoof the attributes used for validation.

[minor] Rejects valid multiline tuple variants with a separate trailing comma

scripts/check_c2s_gate_matrix.py:87 · wiring

A valid Rust tuple variant may place its trailing comma on the next line, for example TupleVariant(\n u8,\n)\n,. After the ) line, tuple_depth is reduced while depth returns to 1; the following comma-only line reaches the top-level parser at lines 87-100, where VARIANT_DECL_RE cannot match it and the checker returns unsupported ClientRequestV1 syntax. The parser tests cover ), on the same line (lines 37-41) but not this valid formatting, so adding or reformatting a tuple variant this way makes the CI gate fail even though Rust accepts the enum.

Root cause: the top-level enum parser treats every non-empty line at enum depth as the start of a variant and has no handling for a standalone trailing comma after a multiline tuple variant. its accepted syntax is therefore narrower than the rust enum syntax the checker claims to support, which can disconnect otherwise valid clientrequestv1 changes from the ci matrix check.

Central review

Decision: infrastructure_failure
Reviewed head: 4734d772dd7bc65536d9752a74de6895e5cdc522
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

The review could not complete safely. No approval or code finding was inferred from the failed stages.

• find:schema-contracts — infra_error: 1 occurrence(s) in 1/2 failed batch(es); samples: batch-0: after 3 attempts: timeout after 300000ms
  • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"}],"stderr":""}

Central review

Decision: request_changes
Reviewed head: 4734d772dd7bc65536d9752a74de6895e5cdc522
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] Nested Rust comments can spoof the enum checked by CI

scripts/check_c2s_gate_matrix.py:13 · security

_mask_rust_non_code relies on RUST_NON_CODE_RE, whose block-comment branch is (?s:/\*.*?\*/). Rust block comments nest, but this non-greedy regex stops at the first inner */. A valid Rust source can therefore put a complete decoy #[serde(...)] pub enum ClientRequestV1 { ... } after an inner comment terminator but before the outer terminator, followed by the real enum with an additional request variant. Rust treats the decoy as commented out, while this checker unmasks it, selects it as the first declaration at line 67, and can report a matching matrix without examining the real wire enum. The tests cover ordinary comments and strings but no nested block comment, so they do not prevent this false pass. This is introduced by the new parser and defeats the PR's security-sensitive enum/matrix CI contract.

Root cause: the checker approximates rust lexical structure with a regular expression that does not implement rust's nested block-comment semantics, allowing commented decoy code to be interpreted as the authoritative enum.

[minor] Unrelated numbered plan tables are parsed as C2S matrix rows

scripts/check_c2s_gate_matrix.py:143 · correctness

matrix_variants() iterates every line in the entire plan and accepts every row matching | <number> | |, without first locating the ## P0 104 变体门禁矩阵 section or stopping at its end. A valid later plan edit that adds another numbered table with backticked identifiers, such as a numbered implementation checklist, is therefore appended to matrix; main() then reports non-contiguous numbering, duplicate/extra variants, or an order mismatch even when the actual C2S matrix still exactly matches ClientRequestV1. No surrounding filter prevents this, and the tests only mock matrix_variants() rather than exercising a plan containing an unrelated numbered table. This behavior is introduced by the new checker and can incorrectly fail the required CI job on unrelated documentation changes.

Root cause: the markdown parser identifies matrix rows solely by a global row-shaped regex and does not scope parsing to the named c2s matrix table or its section boundaries.

[major] Authority contracts are shifted into the matrix's dimension column

docs/plan-refactor-c2s-gate-v1.md:241 · schema-contracts

The matrix header defines the fourth contract column as dimension and its legend says non-spatial requests use —, but AgentUiResponse puts request owner in that dimension column and request target in the ownership/participant column. The real consumer keys AgentUiSessionStore by the authenticated player entity and validates request_id against that player's open session (server/src/network/agent_ui.rs:614-670); there is no request dimension or separate request-target ownership relation. The same column drift appears for other non-spatial session requests, for example BotanyHarvestRequest uses session authority as its dimension at line 147 and CancelExtractRequest does so at line 212. The new CI checker only compares variant names/order/numbers, so these contract cells are not validated.

Root cause: the exhaustive matrix mixes authority-resolution concepts into the dimension column and, for agentuiresponse, shifts ownership terminology into the wrong columns. because p1 is specified to derive gatespec from this matrix, the document does not accurately define the boundary contract for these variants and can produce a missing authenticated-session ownership gate or a nonsensical dimension gate; neither the static checker nor its tests inspect matrix cell semantics.

Suggestions (non-gating)

[suggestion] Avoid running the full E2E pipeline for every documentation change

.github/workflows/e2e.yml:12 · performance

Adding docs/** to the push and pull_request path filters makes every documentation-only change start the full e2e job. That job runs client Gradle tests, dependency setup, a release server build, server tests, smoke/E2E, multiple bot scenarios, and has a 45-minute timeout (.github/workflows/e2e.yml:29, :101-163). The matrix checker itself is only a small static Python check (:120-123), so a docs-only edit now consumes a full CI runner and can add the complete E2E duration to documentation changes.

Root cause: the workflow-level path filter was broadened to include all documentation without separating the lightweight matrix-contract validation from the expensive e2e job. consequently, unrelated documentation edits trigger the entire resource-intensive pipeline.

Central review

Decision: request_changes
Reviewed head: e67b1164b6454fecf640d4859388ee9b86d45b4e
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] External container moves omit the world target's dimension gate

docs/plan-refactor-c2s-gate-v1.md:144 · wiring

The new authority note explicitly assigns ExternalContainerMove a — dimension rule and says its ExternalSession profile is distance-only (lines 140-144; row 81 repeats this at line 228). In production, a session maps to a world entity (ExternalContainerRegistry.sessions), and handle_external_container_move resolves that entity and authorizes ownership via ext.opened_by; however, dimension/reach revalidation is performed only when source_kind is SupplyCoffin (server/src/network/client_request_handler.rs:18234-18275). Generic storage-crate/dead-drop sessions proceed to inventory mutation without checking the player against the target entity's CurrentDimension. A player can therefore open one, change dimensions, and submit the still-owned session ID; even a future distance-only gate can pass when coordinates coincide across dimensions. The target entity already carries CurrentDimension (server/src/world/container_open.rs:58,89-97), so no surrounding invariant prevents this. Because P1 is directed to implement this matrix as the exhaustive gate contract, the newly frozen — rule preserves the reachable cross-dimension mutation rather than closing it.

Root cause: the plan treats authenticated session ownership as sufficient authority for a session tied to a world entity and omits the target entity's dimension from externalcontainermove's gate contract. ownership does not prove that the requester remains in the container's dimension after the session was opened.

[major] Pre-dispatch state gate loses same-update forge request ordering

docs/plan-refactor-c2s-gate-v1.md:89 · concurrency-atomicity

The new plan fixes the gate before event dispatch (docs/plan-refactor-c2s-gate-v1.md:80-89) and gives ForgeTemperingHit a phase/timing state prerequisite while ForgeStepAdvance requires the current step to be complete (:237-240), but it defines no per-ingress-batch pending-state overlay. The real handler deliberately maintains pending_forge_steps (server/src/network/client_request_handler.rs:540) and updates it when a ForgeStepAdvance is accepted (:2702-2710); subsequent ForgeTemperingHit requests consult that pending step (:2664-2682) before the forge systems consume the emitted advance. The production contract is pinned by forge_step_advance_then_tempering_hit_same_update_applies_hit_and_pushes_snapshot (server/src/forge/mod.rs:1565-1623) and the inverse-order rejection test (:1789-1847). Under the documented P1 ordering, a gate that reads only ForgeSessions sees the old Billet phase for both requests and rejects the valid advance-then-hit sequence. Relaxing that check would instead admit the explicitly invalid hit-before-advance sequence. The migration comparison/tests proposed at lines 311-313 do not name or preserve this queued transition state, so they do not resolve the contradiction.

Root cause: the plan treats state gating as a snapshot check before dispatch, but some c2s contracts depend on request order within one bevy update, where earlier accepted events have not yet mutated authoritative domain state. it omits the existing pending transition mechanism from the gate model and p1 acceptance criteria.

[major] Offline-mode username impersonation bypasses the frozen coffin owner check

docs/plan-refactor-c2s-gate-v1.md:111 · security

The new plan freezes Lifecycle.character_id as the authority for CoffinBreak and CoffinMenuReclaim and explicitly describes it as a durable owner identity. However, production starts Valence with ConnectionMode::Offline (server/src/main.rs:92-95), joined-player state is loaded by the handshake-supplied Username (server/src/player/mod.rs:218-229), and the lifecycle character ID is reconstructed from that username plus the stored current_char_id (server/src/combat/mod.rs:116-123; server/src/player/state.rs:331-340). Thus, after the real owner disconnects, a client can connect under the same username; the server loads the owner's stored current character UUID and gives the attacker the exact same Lifecycle.character_id. The proposed equality check then authorizes that client to break or reclaim the owner's coffin. None of the proposed missing/empty/inconsistent-owner checks distinguishes an authenticated owner from a same-name offline-mode connection.

Root cause: the plan treats a persistent character identifier loaded through a client-controlled offline-mode username as proof of identity. persistence makes the identifier stable but does not authenticate the connecting principal, so the frozen owner-only boundary is vulnerable to username impersonation. the design needs an authenticated connection/session principal or an explicitly documented trusted-network assumption and a separate unforgeable owner credential before lifecycle.character_id can serve as authorization authority.

[major] Hard-coded 104 heading blocks the next synchronized request variant

scripts/check_c2s_gate_matrix.py:13 · wiring

MATRIX_SECTION_RE only recognizes the literal heading ## P0 104 变体门禁矩阵. The plan explicitly says at docs/plan-refactor-c2s-gate-v1.md:253 that 104 must not become a permanent constant and that later implementation must cover the then-current variant set. A normal addition of variant 105 that updates the Rust enum, adds matrix row 105, and corrects the heading to P0 105 变体门禁矩阵 makes parse_matrix_variants() never enter the matrix and makes the required CI step fail with cannot parse C2S matrix, even though producer and documented registry are synchronized. The tests only use the 104 heading, so this broken evolution path is not covered.

Root cause: the ci checker's matrix discovery is wired to a mutable baseline count instead of a stable section identity (or a parsed/validated dynamic count), contradicting the plan's dynamic all-current-variants contract and blocking the next correctly synchronized enum expansion.

[major] R4 P1 has no owner for its 18 required wire-gap blockers

docs/plan-refactor-c2s-gate-v1.md:253 · strict-maintainability

The P1 contract at this line makes TypeBox ClientRequestV1 the source of truth and declares the 18 TypeBox/generated-schema omissions to be P1 blockers. However, this plan later says R4 must not touch R6-owned wire/client files (docs/plan-refactor-c2s-gate-v1.md:324) and that P1 starts after only R6 P1 (:311, :325). The R6 plan assigns exhaustive C2S schema/sample pinning to R6 P4, not P1 (docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:24), while the master plan excludes agent/ from the refactor scope (docs/plans-skeleton/plan-refactor-master-v1.md:8). Therefore the documented PR-2/P1 entry condition can be reached after R6 P1 while all 18 declared blockers remain, and neither R4 nor the prerequisite phase owns the changes needed to clear them. Following the stated phase order leaves P1 unable to satisfy its own acceptance contract.

Root cause: the plan declares cross-stack typebox/generated-schema synchronization as an r4 p1 blocker but assigns no prerequisite producer for that work: r4 disclaims the affected boundary, r6 does not schedule it until p4, and r4 waits only for r6 p1. the phase dependency and ownership graph are therefore internally inconsistent.

[major] Fail-closed test misses trailing syntax on supported variant forms

scripts/tests/check_c2s_gate_matrix_test.py:54 · testing

The only fail-closed regression at lines 54-61 uses a unit variant with Unsupported = 1. It does not cover trailing syntax on supported tuple/struct forms. In parse_enum_variants, a declaration such as TupleVariant(u8) = 1, starts with (, is appended as a valid variant, leaves tuple_depth == 0, and the remainder = 1, is never validated. This concrete incorrect parser therefore returns ["TupleVariant"] and all current tests still pass, despite the plan and test summary claiming unknown top-level enum syntax is rejected fail-closed. Such a data-bearing discriminant is valid Rust when the enum has an appropriate #[repr(...)], so compilation does not necessarily prevent the path.

Root cause: the negative parser tests exercise unsupported syntax only when it immediately follows the variant name; they omit trailing tokens after an otherwise supported tuple or struct declaration, matching a parser branch that validates only the first suffix character and silently ignores the remainder.

[major] Enum parser silently drops additional variants on the same line

scripts/check_c2s_gate_matrix.py:172 · schema-contracts

A valid Rust declaration such as pub enum ClientRequestV1 { Alpha, Beta, } reaches this branch with code == "Alpha, Beta,". VARIANT_DECL_RE captures only Alpha, and the suffix check accepts the string merely because its first character is ,; the remaining Beta, is never parsed or rejected. The function therefore returns ["Alpha"], so an unchanged one-row matrix also makes main() return success even though the production enum has gained Beta. The same loss occurs when a tuple closes and another variant follows on that line, because lines handled by the tuple_depth branch are consumed wholesale. Rust permits these layouts, and this workflow does not run or require rustfmt before the checker. Existing tests cover one variant per line and unsupported = 1 syntax, but no valid line containing multiple variants, so they do not expose the false pass. This PR introduces the checker as the CI enforcement mechanism, and the plan explicitly requires unknown top-level syntax to fail closed and exhaustive enum/matrix synchronization.

Root cause: the parser validates only the first character of the text following a captured variant name and does not require the entire top-level declaration fragment to be consumed. consequently, additional valid variants on the same physical line are silently discarded instead of parsed or rejected, allowing the schema/matrix contract gate to pass with an incomplete matrix.

Suggestions (non-gating)

[suggestion] Scope the full E2E trigger to the matrix plan document

.github/workflows/e2e.yml:13 · performance

The new docs/** path filter applies to both push and pull-request triggers, while this job has a 45-minute timeout and proceeds through Node/Rust/Java setup, dependency installation, Gradle tests, Docker/Redis, schema and agent tests, a release server build, server tests, smoke tests, and bot E2E. The repository contains hundreds of Markdown documents, but the new matrix check consumes only docs/plan-refactor-c2s-gate-v1.md. Consequently, an unrelated documentation-only change such as an architecture or finished-plan edit now starts the entire E2E job even though it cannot affect the checked matrix or any runtime artifact. Narrowing the filter to the consumed plan path (and retaining scripts/** and the Rust enum path already covered by server/**) would preserve the contract check without that CI load.

Root cause: the workflow uses the repository-wide docs/** glob to make one plan document participate in the c2s matrix contract, coupling every unrelated documentation edit to the most resource-intensive ci job.

Central review

Decision: request_changes
Reviewed head: 48be57861d9e479460c3670cd12bcfe2d9a78081
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] Reconcile the R4/R6 phase gate before scheduling implementation

docs/plan-refactor-c2s-gate-v1.md:328 · concurrency-atomicity

The new R4 plan states that “P1-P4” cannot start until #1287 + R6 P4 are merged (docs/plan-refactor-c2s-gate-v1.md:328), and its PR-2 acceptance repeats the R6 P4 dependency (docs/plan-refactor-c2s-gate-v1.md:350). The authoritative master plan still schedules Wave 2 R4 after #1287 + R6 P1 (docs/plans-skeleton/plan-refactor-master-v1.md:55). Thus the same refactor track has two incompatible release gates: the master can authorize R4 implementation while the new plan says it is blocked until the later R6 P4 contract, including the 18 C2S wire gaps and coffin credential wire. A scheduler following the master can start R4 before the dependencies that the new plan explicitly requires, producing an implementation that cannot satisfy its own P1 acceptance criteria.

Root cause: the dependency was tightened from the master plan's r6 p1 boundary to r6 p4 in the new r4 plan, but the master plan's wave 2 dependency was not updated or otherwise reconciled. the two plans therefore provide conflicting authoritative sequencing and acceptance gates.

[minor] Docs-only changes now run the full 45-minute E2E job

.github/workflows/e2e.yml:12 · performance

A pull request that changes only docs/** now matches the workflow path filter at .github/workflows/e2e.yml:12 and runs the entire e2e job, including Node/Rust/Java setup, dependency installation, client Gradle tests, Redis image/service startup, server release build/tests, smoke E2E, bot E2E, and chat-window E2E (.github/workflows/e2e.yml:41-163). Before this change, docs-only changes did not trigger this workflow. The result is a full roughly 45-minute runner allocation for documentation changes, even though the newly added matrix checker is a standalone static check.

Root cause: the documentation path was added to the path filters of the heavyweight end-to-end workflow rather than isolating the matrix contract check in a lightweight job or workflow. this makes every docs-only update consume the full e2e build, service, and scenario budget, creating a reproducible ci latency and runner-resource regression.

[major] Malformed matrix rows can bypass the CI contract check

scripts/check_c2s_gate_matrix.py:229 · schema-contracts

parse_matrix_variants only appends lines matching MATRIX_RE at line 229 and silently skips every other table row after the matrix header. For example, a matrix headed P0 2 containing | 1 | \Alpha` | ..., a malformed or mistyped row such as | x | `Gamma` | ..., and then | 2 | `Beta` | ...yields the two parsed rowsAlpha, Beta`; the heading count, contiguous numbering, set, and order checks all pass even though the documented matrix contains an unvalidated extra row. The added tests cover missing, duplicate, and non-contiguous matched rows, but no malformed-row rejection.

Root cause: the checker treats non-matching content inside the selected matrix table as ignorable instead of validating the table boundary and requiring every non-divider row to match the expected schema. consequently, the ci contract step can report a green rust-to-markdown comparison while malformed or extra matrix entries are present.

[major] Serde contract check always rejects the valid enum

scripts/check_c2s_gate_matrix.py:175 · concurrency-atomicity

parse_enum_variants() searches SERDE_ATTR_RE against masked, but _mask_rust_non_code() replaces every quoted string with spaces, including the required serde values. For the real declaration #[serde(deny_unknown_fields, tag = "type", rename_all = "snake_case")], the masked line no longer contains "type" or "snake_case", so no serde attribute matches and serde remains None; the function raises RuntimeError("ClientRequestV1 must use serde tag = \"type\"") before parsing any variants. The parser unit tests all pass source with these quoted serde values, so they fail for the same reason, and the workflow's newly added checker consequently exits nonzero for the repository's valid enum/matrix.

Root cause: the implementation uses a comment/string-masked representation to locate a syntax contract whose identifying values are themselves string literals, then only reads the original source after the match. because masking removes those values before matching, valid serde attributes are indistinguishable from invalid/missing ones.